-
Notifications
You must be signed in to change notification settings - Fork 922
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement ApplicationHandler::create|destroy_surfaces()
#3765
Implement ApplicationHandler::create|destroy_surfaces()
#3765
Conversation
515b6fe
to
66f752a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like there's still a lot to do here to make this correct, and a lot more to make it ergonomic, but splitting these events from each other is a great first step!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like that it called create_surfaces
and not can_create_surfaces
, since the first name kind of forces you to create them.
destroy is fine, because it should force you.
src/changelog/unreleased.md
Outdated
- `ApplicationHandler::create|destroy_surfaces()` was split off from | ||
`ApplicationHandler::resumed/suspended()`. | ||
|
||
`ApplicationHandler::create_surfaces()` should, for portability reasons to | ||
Android, be the only place to create render surfaces. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document that the old behavior of Resumed
is followed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean exactly?
Couple of lines below it the new behavior of resumed/suspended()
is explained.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably write it more clear that the resume
logic should be moved to the new function and that resumed
serves now completely different purpose, because before we advertised it as a place to create a surfaces.
Alternative is to rename resumed to something else, so users don't have to read into the changelog a lot about why their app is not working anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd probably write it more clear that the
resume
logic should be moved to the new function and thatresumed
serves now completely different purpose, because before we advertised it as a place to create a surfaces.
That's not really correct though, the current resume()
functionality isn't new, its just split off. So users can't just move their resume()
logic to can_create_surfaces()
, they have to actually just split it off, like it says in the changelog already.
I'm happy to review a suggestion on what you have in mind though.
Alternative is to rename resumed to something else, so users don't have to read into the changelog a lot about why their app is not working anymore.
I don't think we have to worry about that because it will fail to build anyway unless can_create_surfaces()
is implemented. Hopefully at that point users should be looking at the documentation to implement it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, that's a good point, probably fine then.
@@ -304,10 +304,10 @@ impl EventLoop { | |||
|
|||
app.new_events(&self.window_target, cause); | |||
|
|||
// NB: For consistency all platforms must emit a 'resumed' event even though Wayland | |||
// NB: For consistency all platforms must call `create_surfaces` even though Wayland | |||
// applications don't themselves have a formal suspend/resume lifecycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suspend/resume
part should be reworded. Probably should say that it doesn't limit where it can be created/destroyed.
src/platform_impl/linux/x11/mod.rs
Outdated
@@ -509,10 +509,10 @@ impl EventLoop { | |||
fn single_iteration<A: ApplicationHandler>(&mut self, app: &mut A, cause: StartCause) { | |||
app.new_events(&self.event_processor.target, cause); | |||
|
|||
// NB: For consistency all platforms must emit a 'resumed' event even though X11 | |||
// NB: For consistency all platforms must call `create_surfaces` even though X11 | |||
// applications don't themselves have a formal suspend/resume lifecycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -438,9 +438,11 @@ impl Shared { | |||
} | |||
|
|||
pub fn init(&self) { | |||
// NB: For consistency all platforms must emit a 'resumed' event even though web | |||
// NB: For consistency all platforms must call `create_surfaces` even though web | |||
// applications don't themselves have a formal suspend/resume lifecycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto, though it likely has some cycle, because you have both resume/suspend.
@@ -345,10 +345,10 @@ impl EventLoopRunner { | |||
}, | |||
}; | |||
self.call_event_handler(Event::NewEvents(start_cause)); | |||
// NB: For consistency all platforms must emit a 'resumed' event even though Windows | |||
// NB: For consistency all platforms must call `create_surfaces` even though Windows | |||
// applications don't themselves have a formal suspend/resume lifecycle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
src/application.rs
Outdated
/// | ||
/// [`create_surfaces()`]: Self::create_surfaces | ||
/// [`destroy_surfaces()`]: Self::destroy_surfaces | ||
fn create_surfaces(&mut self, event_loop: &ActiveEventLoop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I entirely like this method, but the alternative is to store windows in winit and indicate the creation with WindowId
.
Though, unless the event loop itself could be less backend agnostic I don't think it'll happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't about Window
anymore, but only about RawWindowHandle
.
The hope is that in the future we won't need this anymore if #3695 is resolved, but yeah, otherwise I can't come up with a better idea.
66f752a
to
c3000dd
Compare
c3000dd
to
e7ae0b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a tad unfortunate that this was created & merged over the weekend, since I only now during the working-week have had time to look at it and have quite a few comments, on the documentation wording and where the docs should be recommending users to place their window creation.
Nothing that can't be fixed in a followup, but it'd have been easier to deal with in one coherent PR, specifically since my input was requested in the last meeting.
fn resumed(&mut self, event_loop: &ActiveEventLoop) { | ||
fn can_create_surfaces(&mut self, event_loop: &ActiveEventLoop) { | ||
let window_attributes = Window::default_attributes().with_title("A fantastic window!"); | ||
self.window = Some(event_loop.create_window(window_attributes).unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically window creation is something that happens before a surface can be created; this changed API just represents the async nature of it (and IIRC @kchibisov said it works like that on Wayland as well). Perhaps our examples shouldn't demonstrate creating window(s) here.
(Especially if we figure out how to make winit::Window <=> android::Activity
, where there could be multiple Activity
s contemporarily)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I will address this in the follow-up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daxpedda did you ever make a followup for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so.
//! impl ApplicationHandler for App { | ||
//! fn resumed(&mut self, event_loop: &ActiveEventLoop) { | ||
//! fn can_create_surfaces(&mut self, event_loop: &ActiveEventLoop) { | ||
//! self.window = Some(event_loop.create_window(Window::default_attributes()).unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also advocates creating a window after its render surface became available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, same as #3765 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I apologize for not letting you review this before merging first!
Considering I was trying not to change anything about the documentation or the implementation outside of what we discussed, I assumed this should be pretty straightforward. Clearly I was wrong!
fn resumed(&mut self, event_loop: &ActiveEventLoop) { | ||
fn can_create_surfaces(&mut self, event_loop: &ActiveEventLoop) { | ||
let window_attributes = Window::default_attributes().with_title("A fantastic window!"); | ||
self.window = Some(event_loop.create_window(window_attributes).unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I will address this in the follow-up PR.
//! impl ApplicationHandler for App { | ||
//! fn resumed(&mut self, event_loop: &ActiveEventLoop) { | ||
//! fn can_create_surfaces(&mut self, event_loop: &ActiveEventLoop) { | ||
//! self.window = Some(event_loop.create_window(Window::default_attributes()).unwrap()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, same as #3765 (comment).
ApplicationHandler::resumed/suspended()
s meaning has become unclear after iOS and Web implemented them.For iOS and Web they specifically mean suspending and resuming the application.
For Android it specifically meant that the render surface will become invalid when suspended and must be recreated when resumed.
This PR introduces
ApplicationHandler::create|destroy_surfaces()
, which take the place ofApplicationHandler::resumed/suspended()
. iOS and Web are now the only ones implementingApplicationHandler::resumed/suspended()
.Fixes #3699.